Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: eth json RPC: eth Filter IDs must act like ints with no leading 0x00 zeros #10261

Closed
wants to merge 2 commits into from

Conversation

snissn
Copy link
Contributor

@snissn snissn commented Feb 13, 2023

Related Issues

#10170

Proposed Changes

change serialization of ethFilterID type to remove leading zeros

Additional Info

itests/eth_conformance_test.go was flaky failing some of the time and this should fix it

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

…ization without any leading zeros and include a test
@snissn snissn requested a review from a team as a code owner February 13, 2023 20:14
@snissn
Copy link
Contributor Author

snissn commented Feb 13, 2023

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more related to the "no leading zeros" requirement, i.e. "compact hex" encoding thing.

@snissn snissn marked this pull request as draft February 13, 2023 23:08
@snissn
Copy link
Contributor Author

snissn commented Feb 13, 2023

@raulk Yeah I think the leading zeros thing can be focused around to just the EthFilterID type - which seems to be generated by a hash, but the jsonrpc is expecting a data format more like an int without leading zeros. There's sort of two heads driving the car (or whatever metaphor makes sense) in the sense that the eth filter json rpc spec seems to imply that there should be no leading zeros but other consumers currently seem to expect it to be more like a hash which allows for leading zeros ie
this test that's failing in this draft PR: https://app.circleci.com/pipelines/github/filecoin-project/lotus/26527/workflows/d1125d69-1021-46d4-b554-4e6b0d75fe1f/jobs/837757 RPC client error: unmarshaling result: expected hex string length sans prefix 64, got 62

@snissn snissn mentioned this pull request Feb 13, 2023
7 tasks
@iand
Copy link
Contributor

iand commented Feb 14, 2023

Should subscriptionID be treated the same way?

@jennijuju jennijuju added this to the Network v18 milestone Feb 16, 2023
@arajasek
Copy link
Contributor

Closing this until we have a clear direction / get user feedback on the need. Test being tweaked in #10297

@arajasek arajasek closed this Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants